-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[IVDesc] Use SCEVPatternMatch to improve code (NFC) #168397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-vectorizers Author: Ramkumar Ramachandra (artagnon) ChangesFull diff: https://github.com/llvm/llvm-project/pull/168397.diff 4 Files Affected:
diff --git a/llvm/include/llvm/Analysis/IVDescriptors.h b/llvm/include/llvm/Analysis/IVDescriptors.h
index 654a5f10cea96..2b7707db9d16b 100644
--- a/llvm/include/llvm/Analysis/IVDescriptors.h
+++ b/llvm/include/llvm/Analysis/IVDescriptors.h
@@ -401,7 +401,7 @@ class InductionDescriptor {
InductionKind getKind() const { return IK; }
const SCEV *getStep() const { return Step; }
BinaryOperator *getInductionBinOp() const { return InductionBinOp; }
- LLVM_ABI ConstantInt *getConstIntStepValue() const;
+ LLVM_ABI const APInt *getStepValue() const;
/// Returns true if \p Phi is an induction in the loop \p L. If \p Phi is an
/// induction, the induction descriptor \p D will contain the data describing
diff --git a/llvm/lib/Analysis/IVDescriptors.cpp b/llvm/lib/Analysis/IVDescriptors.cpp
index 641850b46bbd8..409a45d6fc542 100644
--- a/llvm/lib/Analysis/IVDescriptors.cpp
+++ b/llvm/lib/Analysis/IVDescriptors.cpp
@@ -15,6 +15,7 @@
#include "llvm/Analysis/LoopInfo.h"
#include "llvm/Analysis/ScalarEvolution.h"
#include "llvm/Analysis/ScalarEvolutionExpressions.h"
+#include "llvm/Analysis/ScalarEvolutionPatternMatch.h"
#include "llvm/Analysis/ValueTracking.h"
#include "llvm/IR/Dominators.h"
#include "llvm/IR/Instructions.h"
@@ -25,6 +26,7 @@
using namespace llvm;
using namespace llvm::PatternMatch;
+using namespace llvm::SCEVPatternMatch;
#define DEBUG_TYPE "iv-descriptors"
@@ -719,11 +721,12 @@ RecurrenceDescriptor::isFindIVPattern(RecurKind Kind, Loop *TheLoop,
if (!SE.isSCEVable(Ty))
return std::nullopt;
- auto *AR = dyn_cast<SCEVAddRecExpr>(SE.getSCEV(V));
- if (!AR || AR->getLoop() != TheLoop)
+ auto *AR = SE.getSCEV(V);
+ const SCEV *Step;
+ if (!match(AR, m_scev_AffineAddRec(m_SCEV(), m_SCEV(Step),
+ m_SpecificLoop(TheLoop))))
return std::nullopt;
- const SCEV *Step = AR->getStepRecurrence(SE);
if ((isFindFirstIVRecurrenceKind(Kind) && !SE.isKnownNegative(Step)) ||
(isFindLastIVRecurrenceKind(Kind) && !SE.isKnownPositive(Step)))
return std::nullopt;
@@ -1377,7 +1380,7 @@ InductionDescriptor::InductionDescriptor(Value *Start, InductionKind K,
"StartValue is not an integer for integer induction");
// Check the Step Value. It should be non-zero integer value.
- assert((!getConstIntStepValue() || !getConstIntStepValue()->isZero()) &&
+ assert((!getStepValue() || !getStepValue()->isZero()) &&
"Step value is zero");
assert((IK == IK_FpInduction || Step->getType()->isIntegerTy()) &&
@@ -1395,10 +1398,11 @@ InductionDescriptor::InductionDescriptor(Value *Start, InductionKind K,
llvm::append_range(RedundantCasts, *Casts);
}
-ConstantInt *InductionDescriptor::getConstIntStepValue() const {
- if (isa<SCEVConstant>(Step))
- return dyn_cast<ConstantInt>(cast<SCEVConstant>(Step)->getValue());
- return nullptr;
+const APInt *InductionDescriptor::getStepValue() const {
+ const APInt *StepC;
+ if (!match(Step, m_scev_APInt(StepC)))
+ return nullptr;
+ return StepC;
}
bool InductionDescriptor::isFPInductionPHI(PHINode *Phi, const Loop *TheLoop,
@@ -1614,41 +1618,26 @@ bool InductionDescriptor::isInductionPHI(
// Check that the PHI is consecutive.
const SCEV *PhiScev = Expr ? Expr : SE->getSCEV(Phi);
- const SCEVAddRecExpr *AR = dyn_cast<SCEVAddRecExpr>(PhiScev);
-
- if (!AR) {
+ const SCEV *Step;
+ if (!match(PhiScev, m_scev_AffineAddRec(m_SCEV(), m_SCEV(Step),
+ m_SpecificLoop(TheLoop)))) {
LLVM_DEBUG(dbgs() << "LV: PHI is not a poly recurrence.\n");
return false;
}
- if (AR->getLoop() != TheLoop) {
- // FIXME: We should treat this as a uniform. Unfortunately, we
- // don't currently know how to handled uniform PHIs.
- LLVM_DEBUG(
- dbgs() << "LV: PHI is a recurrence with respect to an outer loop.\n");
- return false;
- }
-
// This function assumes that InductionPhi is called only on Phi nodes
// present inside loop headers. Check for the same, and throw an assert if
// the current Phi is not present inside the loop header.
- assert(Phi->getParent() == AR->getLoop()->getHeader()
- && "Invalid Phi node, not present in loop header");
+ assert(Phi->getParent() == TheLoop->getHeader() &&
+ "Invalid Phi node, not present in loop header");
Value *StartValue =
- Phi->getIncomingValueForBlock(AR->getLoop()->getLoopPreheader());
+ Phi->getIncomingValueForBlock(TheLoop->getLoopPreheader());
- BasicBlock *Latch = AR->getLoop()->getLoopLatch();
+ BasicBlock *Latch = TheLoop->getLoopLatch();
if (!Latch)
return false;
- const SCEV *Step = AR->getStepRecurrence(*SE);
- // Calculate the pointer stride and check if it is consecutive.
- // The stride may be a constant or a loop invariant integer value.
- const SCEVConstant *ConstStep = dyn_cast<SCEVConstant>(Step);
- if (!ConstStep && !SE->isLoopInvariant(Step, TheLoop))
- return false;
-
if (PhiTy->isIntegerTy()) {
BinaryOperator *BOp =
dyn_cast<BinaryOperator>(Phi->getIncomingValueForBlock(Latch));
diff --git a/llvm/lib/Analysis/LoopInfo.cpp b/llvm/lib/Analysis/LoopInfo.cpp
index d84721b7f8f4b..7f261f21e1e5b 100644
--- a/llvm/lib/Analysis/LoopInfo.cpp
+++ b/llvm/lib/Analysis/LoopInfo.cpp
@@ -421,7 +421,7 @@ bool Loop::isCanonical(ScalarEvolution &SE) const {
if (IndDesc.getInductionOpcode() != Instruction::Add)
return false;
- ConstantInt *Step = IndDesc.getConstIntStepValue();
+ const APInt *Step = IndDesc.getStepValue();
if (!Step || !Step->isOne())
return false;
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
index 03112c67dda7b..bc57ef5e71725 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
@@ -706,7 +706,7 @@ void LoopVectorizationLegality::addInductionPhi(
// Int inductions are special because we only allow one IV.
if (ID.getKind() == InductionDescriptor::IK_IntInduction &&
- ID.getConstIntStepValue() && ID.getConstIntStepValue()->isOne() &&
+ ID.getStepValue() && ID.getStepValue()->isOne() &&
isa<Constant>(ID.getStartValue()) &&
cast<Constant>(ID.getStartValue())->isNullValue()) {
@@ -891,7 +891,7 @@ bool LoopVectorizationLegality::canVectorizeInstr(Instruction &I) {
if (AllowStridedPointerIVs)
return false;
return ID.getKind() == InductionDescriptor::IK_PtrInduction &&
- ID.getConstIntStepValue() == nullptr;
+ ID.getStepValue() == nullptr;
};
// TODO: Instead of recording the AllowedExit, it would be good to
|
| // Calculate the pointer stride and check if it is consecutive. | ||
| // The stride may be a constant or a loop invariant integer value. | ||
| const SCEVConstant *ConstStep = dyn_cast<SCEVConstant>(Step); | ||
| if (!ConstStep && !SE->isLoopInvariant(Step, TheLoop)) | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we drop this? Do m_SCEV(Step) guarantee this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't m_scev_AffineAddRec guarantee this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think m_scev_AffineAddRec guarantee this. It should depend on m_SCEV(Step).
If we use m_SCEVConstant(Step), Step is SCEVConstant. But, we use m_SCEV(Step), so it should only get a SCEV, and not guarantee anything, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From ScalarEvolutionExpressions:
/// Return true if this represents an expression A + B*x where A
/// and B are loop invariant values.
bool isAffine() const {They must be loop-invariant if the AddRec is affine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this makes me wonder if const SCEVAddRecExpr *AR = dyn_cast<SCEVAddRecExpr>(PhiScev); is equivalent to match(PhiScev, m_scev_AffineAddRec()). Could PhiScev originally have been a quadratic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could have, but we'd have bailed out anyway when checking that the step is loop invariant? We're just bailing out ahead of time now, and this should be NFC?
🐧 Linux x64 Test Results
|
| auto *AR = dyn_cast<SCEVAddRecExpr>(SE.getSCEV(V)); | ||
| if (!AR || AR->getLoop() != TheLoop) | ||
| auto *AR = SE.getSCEV(V); | ||
| const SCEV *Step; | ||
| if (!match(AR, m_scev_AffineAddRec(m_SCEV(), m_SCEV(Step), | ||
| m_SpecificLoop(TheLoop)))) | ||
| return std::nullopt; | ||
|
|
||
| const SCEV *Step = AR->getStepRecurrence(SE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code isn't directly equivalent, as we're restricting to affine AddRecs, when we weren't previously. However, I think Find(First|Last)IV wouldn't work with a non-affine AddRec anyway.
No description provided.